Skip to content

Conversation

milmazz
Copy link
Member

@milmazz milmazz commented Feb 13, 2017

No description provided.

@milmazz milmazz force-pushed the ebertapp branch 4 times, most recently from 9c6e567 to 1b69d70 Compare February 13, 2017 21:11
@josevalim
Copy link
Member

@milmazz I would prefer to not have a .credo.exs file. Why do we need one?

Everything else looks beautiful.

@milmazz
Copy link
Member Author

milmazz commented Feb 13, 2017

@josevalim In some cases, we want to ignore some issues, for example:

  • In the directory test/fixtures (e.g. @moduledoc not present for tests)
  • Set 6 as maximum arity instead of 5

wdyt?

@josevalim
Copy link
Member

We should probably fix the arity ones although I am not sure what to do about the test/fixtures. :S Maybe we should use a @lint annotation on those?

@milmazz
Copy link
Member Author

milmazz commented Feb 14, 2017

We should probably fix the arity ones

@josevalim I agree, I can do that in another PR/commit.

I am not sure what to do about the test/fixtures. :S Maybe we should use a @lint annotation on those?

I think we cannot use the inline configuration via @lint attributes at the top level defmodule, but, it should work on the nested ones. @rrrene Can you confirm this?

@rrrene
Copy link

rrrene commented Feb 14, 2017

@milmazz You are right. @lint does not work on the module level.

I just decided to deprecate the use of @lint in favor of a comment-based format. There is a discussion in rrrene/credo#291 where @josevalim rightly pointed out that maybe @lint attributes should not be module attributes since they are not executable code. After thinking about it, I tend to agree.

Now we have to figure out a better syntax for the "magic control comments", but I am optimistic that we can find an elegant solution. Everybody's input on this would be very welcome 👍

@josevalim
Copy link
Member

Thanks @rrrene.

@milmazz while I would personally prefer to not have a .credo.exs since when a new credo version is released I will need to update many repos instead of a single source, feel free to proceed as you prefer.

@sourcelevel-bot
Copy link

Ebert has finished reviewing this Pull Request and has found:

  • 10 fixed issues! 🎉

But beware that this branch is 2 commits behind the elixir-lang:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/elixir-lang/ex_doc/pulls/672.

@milmazz milmazz merged commit 05eb89e into elixir-lang:master Feb 14, 2017
@milmazz
Copy link
Member Author

milmazz commented Feb 14, 2017

@rrrene Thanks for the info.

@josevalim I already removed the .credo.exs file. That way we can see that we have some pending issues to fix and alleviate the maintenance burden a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants